[mlir][emitc] Simplify inlining logic (NFCI)#169978
Merged
Conversation
Member
|
@llvm/pr-subscribers-mlir-emitc @llvm/pr-subscribers-mlir Author: Gil Rapaport (aniragil) ChangesThis change makes inlining logic in the translator simpler and more consistent by (a) Extending the inlining concept to include CExpression ops, which by (b) Concentraing all inlining decisions in Full diff: https://github.com/llvm/llvm-project/pull/169978.diff 1 Files Affected:
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index bcce0a5145221..3936087ec3293 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -111,6 +111,8 @@ static FailureOr<int> getOperatorPrecedence(Operation *operation) {
.Default([](auto op) { return op->emitError("unsupported operation"); });
}
+static bool shouldBeInlined(Operation *op);
+
namespace {
/// Emitter that uses dialect specific emitters to emit C++ code.
struct CppEmitter {
@@ -254,24 +256,19 @@ struct CppEmitter {
}
/// Is expression currently being emitted.
- bool isEmittingExpression() { return emittedExpression; }
+ bool isEmittingExpression() { return !emittedExpressionPrecedence.empty(); }
/// Determine whether given value is part of the expression potentially being
/// emitted.
bool isPartOfCurrentExpression(Value value) {
- if (!emittedExpression)
- return false;
Operation *def = value.getDefiningOp();
- if (!def)
- return false;
- return isPartOfCurrentExpression(def);
+ return def ? isPartOfCurrentExpression(def) : false;
}
/// Determine whether given operation is part of the expression potentially
/// being emitted.
bool isPartOfCurrentExpression(Operation *def) {
- auto operandExpression = dyn_cast<ExpressionOp>(def->getParentOp());
- return operandExpression && operandExpression == emittedExpression;
+ return isEmittingExpression() && shouldBeInlined(def);
};
// Resets the value counter to 0.
@@ -318,7 +315,6 @@ struct CppEmitter {
unsigned int valueCount{0};
/// State of the current expression being emitted.
- ExpressionOp emittedExpression;
SmallVector<int> emittedExpressionPrecedence;
void pushExpressionPrecedence(int precedence) {
@@ -341,12 +337,22 @@ static bool hasDeferredEmission(Operation *op) {
emitc::GetFieldOp>(op);
}
-/// Determine whether expression \p expressionOp should be emitted inline, i.e.
+/// Determine whether operation \p op should be emitted inline, i.e.
/// as part of its user. This function recommends inlining of any expressions
/// that can be inlined unless it is used by another expression, under the
/// assumption that any expression fusion/re-materialization was taken care of
/// by transformations run by the backend.
-static bool shouldBeInlined(ExpressionOp expressionOp) {
+static bool shouldBeInlined(Operation *op) {
+ // CExpression operations are inlined if and only if they reside within an
+ // ExpressionOp.
+ if (isa<CExpressionInterface>(op))
+ return isa<ExpressionOp>(op->getParentOp());
+
+ // Only other inlinable operation is ExpressionOp itself.
+ ExpressionOp expressionOp = dyn_cast<ExpressionOp>(op);
+ if (!expressionOp)
+ return false;
+
// Do not inline if expression is marked as such.
if (expressionOp.getDoNotInline())
return false;
@@ -1564,7 +1570,6 @@ LogicalResult CppEmitter::emitExpression(ExpressionOp expressionOp) {
"Expected precedence stack to be empty");
Operation *rootOp = expressionOp.getRootOp();
- emittedExpression = expressionOp;
FailureOr<int> precedence = getOperatorPrecedence(rootOp);
if (failed(precedence))
return failure();
@@ -1576,7 +1581,6 @@ LogicalResult CppEmitter::emitExpression(ExpressionOp expressionOp) {
popExpressionPrecedence();
assert(emittedExpressionPrecedence.empty() &&
"Expected precedence stack to be empty");
- emittedExpression = nullptr;
return success();
}
@@ -1617,14 +1621,8 @@ LogicalResult CppEmitter::emitOperand(Value value, bool isInBrackets) {
// If this operand is a block argument of an expression, emit instead the
// matching expression parameter.
Operation *argOp = arg.getParentBlock()->getParentOp();
- if (auto expressionOp = dyn_cast<ExpressionOp>(argOp)) {
- // This scenario is only expected when one of the operations within the
- // expression being emitted references one of the expression's block
- // arguments.
- assert(expressionOp == emittedExpression &&
- "Expected expression being emitted");
- value = expressionOp->getOperand(arg.getArgNumber());
- }
+ if (auto expressionOp = dyn_cast<ExpressionOp>(argOp))
+ return emitOperand(expressionOp->getOperand(arg.getArgNumber()));
}
os << getOrCreateName(value);
|
This change makes inlining logic in the translator simpler and more
consistent by
(a) Extending the inlining concept to include CExpression ops, which by
definition are inlined if and only if they reside within an
ExpressionOp.
(b) Concentraing all inlining decisions in `shouldBeInlined()` to make
sure that ops get the same decision when queried as operations and
as operands.
cb5a2fb to
723dbac
Compare
Contributor
Author
|
ping |
simon-camp
approved these changes
Dec 7, 2025
honeygoyal
pushed a commit
to honeygoyal/llvm-project
that referenced
this pull request
Dec 9, 2025
This change makes inlining logic in the translator simpler and more
consistent by
(a) Extending the inlining concept to include CExpression ops, which by
definition are inlined if and only if they reside within an
ExpressionOp.
(b) Concentraing all inlining decisions in `shouldBeInlined()` to make
sure that ops get the same decision when queried as operations and
as operands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change makes inlining logic in the translator simpler and more consistent by
(a) Extending the inlining concept to include CExpression ops, which by
definition are inlined if and only if they reside within an
ExpressionOp.
(b) Concentraing all inlining decisions in
shouldBeInlined()to makesure that ops get the same decision when queried as operations and
as operands.